feat(new transform): Aggregate transform to reduce metric volume maintaining data #7846

ross · 2021-06-11T20:25:33Z

The transform combines (add's) Incremental metric events and uses "last one wins" semantics for Aggregates. This is similar to how statsd works, but it does not (yet) support things like continuing to emit 0's for metrics it's previously seen.

Testing
TODO discussion & address
Documentation
Anything else that comes up in discussion and review

/cc #676 which requested statsd functionality
/cc #3668 which is functionality that could be built on top of this PR if desired.

Signed-off-by: Ross McFarland <[email protected]>

docs/manual/setup/installation/manual/from-source.md

Signed-off-by: Ross McFarland <[email protected]>

spencergilbert · 2021-06-14T20:55:04Z

Seems reasonable to me, but I'll leave the TODO's/questions to others

tobz

Overall this is looking pretty good, but a few things are jumping out at the moment.

src/transforms/aggregate.rs

Signed-off-by: Ross McFarland <[email protected]>

tobz

Getting closer!

src/transforms/aggregate.rs

Signed-off-by: Ross McFarland <[email protected]>

tobz

Overall, this looks good. There's still the open question about event metadata which I'll get an answer to, but other than that, we just need to nail down the performance/polish.

src/transforms/aggregate.rs

docs/reference/components/transforms/aggregate.cue

src/transforms/aggregate.rs

Signed-off-by: Ross McFarland <[email protected]>

ross

Most of the changes done. There's a couple where there's still outstanding questions before I could do anything with them. I think this is pretty shippable as-is.

src/transforms/aggregate.rs

ross · 2021-06-18T23:27:12Z

src/transforms/aggregate.rs

+ where
+ Self: 'static,
+ {
+ let mut me = self;


src/transforms/aggregate.rs

ross · 2021-06-18T23:52:06Z

src/transforms/aggregate.rs

+ metric::MetricKind::Incremental => {
+ match self.map.get_mut(&series) {
+ // We already have something, add to it, will update timestamp as well.
+ Some(existing) => existing.update(data),


Now emit!s an internal metric when this happens.

src/transforms/aggregate.rs

ross · 2021-06-19T00:16:19Z

For some of the sinks, it is simply not possible to avoid. In particular, the Prometheus text and remote_write formats can only expose one metric per time series, and so they require some aggregation before that. The prometheus_exporter sink avoids using one of the standard buffers entirely because it handles it in a different way internally.

Yeah it makes sense when there's a requirement/limitation downstream as described here, but still don't think I'd want it to be happening implicitly when not forced to.

I agree this transform is valuable to Vector, and I hope my discussion did not imply otherwise. The ability to use it in a transform pipeline is indeed an advantage.

🆒

bruceg

This looks much better. Down to a couple of minor tweaks from me.

src/transforms/aggregate.rs

bruceg · 2021-06-21T19:28:03Z

src/transforms/aggregate.rs

+ }
+
+ fn flush_into(&mut self, output: &mut Vec<Event>) {
+ if self.map.len() > 0 {


Does this condition add any value? If the len is zero, the map drain will be a no-op too. I would also expect clippy to gripe about this should use is_empty.

I'd assume it's super minor if at all. It was more meaningful back when the code was swapping out maps. Will remove.

docs/reference/components/transforms/aggregate.cue

Signed-off-by: Ross McFarland <[email protected]>

Also rustfmt Signed-off-by: Jesse Szwedko <[email protected]>

Signed-off-by: Ross McFarland <[email protected]>

bruceg

I think I am good with this except for the question about handling update failures.

bruceg · 2021-06-23T19:03:58Z

src/transforms/aggregate.rs

+ if !existing.0.update(&data) {
+ emit!(AggregateUpdateFailed);
+ }
+ existing.1.merge(metadata);


In other sinks, when the update fails, we drop the older metric instead of the newer one as is being done here. I think that would be the more sensible behavior, but I am open to your thoughts. Suggestion:

Suggested change

if !existing.0.update(&data) {

emit!(AggregateUpdateFailed);

}

existing.1.merge(metadata);

if !existing.0.update(&data) {

emit!(AggregateUpdateFailed);

*existing = (data, metadata);

} else {

existing.1.merge(metadata);

}

Note that the assignment causes an implicit drop of the metadata, which is reasonable given that we are also dropping the data as well.

Ok. Didn't know the precedent and in a vacuum either option seemed equally bad so I went with the simpler one. Will update now and add in a test for the behavior. Also removed the ! and flipped the branches to avoid the non-essential not.

Actually @bruceg, it looks like MetricData.update succeeds when the kind changes (glad i tried to write tests for that case) so that code is "add"ing Incremental and Absolute values which doesn't really make sense. I looked into the metrics buffer and I think it's checking for type matches before doing the add. Kind of seems like update should do the add or replace logic based on kind so that all the users of that method don't have to check and handle that case, but for now I'll rework this to check kind first.

Ok. Change made in the spirit of your request and the edge cases are now tested and behaving as expected/intended.

Yes, it is a new pattern so it wouldn't have been obvious.

Good catch, yes update doesn't check the metric kind. It looks like the logic this transform wants and the logic for the metrics buffer is different enough that I'm not clear what the proper semantics should be. I'll have to give that more thought.

If nothing else it could fail the update, do nothing and return false. Assuming something elsewhere isn't expecting/wanting it to add things with non-matching kind. That would let aggregate use it as is. Would have to dig deeper to see what metrics buffer is doing, but I don't think failing the update would hurt it since it would check before sending it and never get there in that case.

Yeah, I was thinking more along the lines of update possibly taking the new data by value instead of reference and returning it back in the case of an error as Result<(), MetricData>, to avoid needing to clone data inside update. On the other hand, the metrics buffer wants to be able to do add incrementals to both kinds, while this transform wants to replace for the case of different kinds, so that's hard to shoehorn into one method.

Guess it's not clear to me how adding Absolutes makes sense given they're intended to be the value as-is, e.g. if you're going 55mph at t0 and 57mpg at t1, 112mph isn't a meaningful number. It seems like kind and Counter/Guage kind of overlap semantic wise and can result in internally conflicting states.

Anyway, not really related to this PR so I can let it go 😁

No, you can't add absolutes, but you can update an absolute to a new absolute value. You can add an incremental to either kind as well. Agreed it's not related to this PR.

…kind and value types Signed-off-by: Ross McFarland <[email protected]>

Signed-off-by: Ross McFarland <[email protected]>

tobz · 2021-06-24T18:53:18Z

95 comments, 36 commits... hooo boy. :)

Appreciate you sticking with us on this one. 👍🏻

binarylogic · 2021-06-25T22:12:49Z

Thank you @ross! This is a great contribution. As @tobz, appreciate you sticking with us.

jszwedko · 2021-06-25T22:18:32Z

docs/reference/components/transforms/aggregate.cue

+
+ examples: [
+ {
+ title: "Aggregate over 15 seconds"


Just noting that this says 15 seconds but the interval_ms below is 5000.

ross added 4 commits June 11, 2021 07:20

WIP: exploration of aggregate transform

c7556ac

Signed-off-by: Ross McFarland <[email protected]>

Rework plumbing based on Reduce to allow async/timer generation

6b5126f

Signed-off-by: Ross McFarland <[email protected]>

Working a/b maps for Aggregate

3475e46

Signed-off-by: Ross McFarland <[email protected]>

Working statsd style aggregation

f826e3a

Signed-off-by: Ross McFarland <[email protected]>

ross force-pushed the transforms-aggregate branch from 9bdc6e7 to f826e3a Compare June 12, 2021 02:24

ross added 3 commits June 11, 2021 19:26

Add tests for aggregate and rework/fix bits found

e1c6e08

Signed-off-by: Ross McFarland <[email protected]>

Use MetricData::update, we already know it's incremental

c135e8e

Signed-off-by: Ross McFarland <[email protected]>

Documentation for aggregate transform

010d9d6

Signed-off-by: Ross McFarland <[email protected]>

ross marked this pull request as ready for review June 13, 2021 21:36

ross requested review from a team, bruceg and pablosichert and removed request for a team June 13, 2021 21:36

ross added 2 commits June 13, 2021 16:39

cue fixes

35cdb39

Signed-off-by: Ross McFarland <[email protected]>

Remove , and fix tab/spaces in aggregate cue doc

9f3c2b9

Signed-off-by: Ross McFarland <[email protected]>

spencergilbert reviewed Jun 14, 2021

View reviewed changes

docs/manual/setup/installation/manual/from-source.md Outdated Show resolved Hide resolved

ross added 2 commits June 14, 2021 13:34

Fix aggregate/add_tags copy-n-paste error

e159961

Signed-off-by: Ross McFarland <[email protected]>

More aggregate cue fixes, this time from CI=true make check-docs

f3118ac

Signed-off-by: Ross McFarland <[email protected]>

tobz requested changes Jun 15, 2021

View reviewed changes

src/transforms/aggregate.rs Outdated Show resolved Hide resolved

src/transforms/aggregate.rs Outdated Show resolved Hide resolved

Use tokio::time::advance to force interval in aggregate test

1da1e06

Signed-off-by: Ross McFarland <[email protected]>

tobz requested changes Jun 15, 2021

View reviewed changes

src/transforms/aggregate.rs Outdated Show resolved Hide resolved

ross added 3 commits June 15, 2021 13:30

aggregate single map

dda9ab6

Signed-off-by: Ross McFarland <[email protected]>

Full testing of aggregate flushing via shutdown and tick

7b73e64

Signed-off-by: Ross McFarland <[email protected]>

Merge branch 'master' into transforms-aggregate

314f021

tobz requested changes Jun 16, 2021

View reviewed changes

ross added 2 commits June 16, 2021 18:25

aggregate: remove local map ref vars

a436354

Signed-off-by: Ross McFarland <[email protected]>

guage -> gauge

2b498bf

Signed-off-by: Ross McFarland <[email protected]>

Store and use event metadata

40b6123

Signed-off-by: Ross McFarland <[email protected]>

ross commented Jun 19, 2021

View reviewed changes

ross requested a review from a team June 19, 2021 00:06

bruceg reviewed Jun 21, 2021

View reviewed changes

ross and others added 4 commits June 22, 2021 08:49

Wording updates and corrections for aggregate doc

fe083c5

Signed-off-by: Ross McFarland <[email protected]>

Avoid clone in update or insert logic for aggregate

8c33d74

Signed-off-by: Ross McFarland <[email protected]>

Move unused import

a027d3f

Also rustfmt Signed-off-by: Jesse Szwedko <[email protected]>

clippy suggested cleanup for aggregate

81c7541

Signed-off-by: Ross McFarland <[email protected]>

jszwedko requested review from bruceg and tobz June 23, 2021 18:49

bruceg reviewed Jun 23, 2021

View reviewed changes

Correctly handle mis-matching kind's and explicitly test conflicting …

ce95db2

…kind and value types Signed-off-by: Ross McFarland <[email protected]>

bruceg approved these changes Jun 24, 2021

View reviewed changes

ross added 2 commits June 23, 2021 21:24

make clippy happy

8e58c2f

Signed-off-by: Ross McFarland <[email protected]>

More clippy fodder

ce84d30

Signed-off-by: Ross McFarland <[email protected]>

tobz approved these changes Jun 24, 2021

View reviewed changes

bruceg enabled auto-merge (squash) June 24, 2021 18:55

bruceg disabled auto-merge June 25, 2021 21:23

bruceg enabled auto-merge (squash) June 25, 2021 21:24

bruceg changed the title ~~enhancement: Aggregate transform to reduce metric volume maintaining data~~ feat(new transform): Aggregate transform to reduce metric volume maintaining data Jun 25, 2021

jszwedko disabled auto-merge June 25, 2021 21:53

jszwedko enabled auto-merge (squash) June 25, 2021 21:55

jszwedko disabled auto-merge June 25, 2021 21:55

jszwedko merged commit fe84032 into vectordotdev:master Jun 25, 2021

jszwedko reviewed Jun 25, 2021

View reviewed changes

ross deleted the transforms-aggregate branch June 25, 2021 22:33

jszwedko mentioned this pull request Jun 10, 2022

Aggegeration of Statsd metrics #676

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(new transform): Aggregate transform to reduce metric volume maintaining data #7846

feat(new transform): Aggregate transform to reduce metric volume maintaining data #7846

ross commented Jun 11, 2021 •

edited

Loading

spencergilbert commented Jun 14, 2021

tobz left a comment

tobz left a comment

tobz left a comment

ross left a comment

ross Jun 18, 2021

ross Jun 18, 2021

ross commented Jun 19, 2021

bruceg left a comment

bruceg Jun 21, 2021

ross Jun 22, 2021

bruceg left a comment

bruceg Jun 23, 2021

ross Jun 23, 2021 •

edited

Loading

ross Jun 23, 2021

ross Jun 24, 2021

bruceg Jun 24, 2021

ross Jun 24, 2021 •

edited

Loading

bruceg Jun 24, 2021

ross Jun 24, 2021

bruceg Jun 24, 2021

tobz commented Jun 24, 2021

binarylogic commented Jun 25, 2021

jszwedko Jun 25, 2021

feat(new transform): Aggregate transform to reduce metric volume maintaining data #7846

feat(new transform): Aggregate transform to reduce metric volume maintaining data #7846

Conversation

ross commented Jun 11, 2021 • edited Loading

spencergilbert commented Jun 14, 2021

tobz left a comment

Choose a reason for hiding this comment

tobz left a comment

Choose a reason for hiding this comment

tobz left a comment

Choose a reason for hiding this comment

ross left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ross commented Jun 19, 2021

bruceg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruceg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ross Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ross Jun 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobz commented Jun 24, 2021

binarylogic commented Jun 25, 2021

Choose a reason for hiding this comment

ross commented Jun 11, 2021 •

edited

Loading

ross Jun 23, 2021 •

edited

Loading

ross Jun 24, 2021 •

edited

Loading